VPN-6749: Fix shared addon ids #10065
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcleinman
requested review from
flodolo,
oskirby,
strseb and
lesleyjanenorton
This was referenced Nov 22, 2024
flodolo
reviewed
Nov 23, 2024
| @@ -59,6 +69,8 @@ def prune_lists_to_strings(data): | |||
| print("First, pull in the strings from the YAML file") | |||
| translation_strings = parseYAMLTranslationStrings(args.infile) | |||
| translation_strings = prune_lists_to_strings(translation_strings) | |||
| # parseYAMLTranslationStrings gives a different ID than we want to use | |||
| translation_strings = use_proper_id(translation_strings) | |||
There was a problem hiding this comment.
Instead of regenerating, have you considered changing write_en_language(). It looks like a cleaner approach to me. This would be the diff compared to your current patch.
diff --git a/scripts/shared.py b/scripts/shared.py
index 46ea19700..5b6222561 100644
--- a/scripts/shared.py
+++ b/scripts/shared.py
@@ -2,7 +2,7 @@ import os
import sys
import xml.etree.ElementTree as ET
-def write_en_language(filename, strings):
+def write_en_language(filename, strings, key_as_id=True):
ts = ET.Element("TS")
ts.set("version", "2.1")
ts.set("language", "en")
@@ -11,8 +11,9 @@ def write_en_language(filename, strings):
ET.SubElement(context, "name")
for key, value in strings.items():
+ id = key if key_as_id else value["string_id"]
message = ET.SubElement(context, "message")
- message.set("id", key)
+ message.set("id", id)
location = ET.SubElement(message, "location")
location.set("filename", "addon.qml")
diff --git a/scripts/utils/generate_shared_addon_xliff.py b/scripts/utils/generate_shared_addon_xliff.py
index 56efdc486..39b2f91e2 100644
--- a/scripts/utils/generate_shared_addon_xliff.py
+++ b/scripts/utils/generate_shared_addon_xliff.py
@@ -28,16 +28,6 @@ def prune_lists_to_strings(data):
sys.exit("Unexpected input")
return data
-def use_proper_id(data):
- return_data = {}
- for value in data.values():
- string_id = value["string_id"]
- return_data[string_id] = {
- "value": value["value"],
- "comments": value["comments"]
- }
- return return_data
-
# Make sure we have all the required things
# Lookup our required tools for addon generation.
@@ -69,13 +59,11 @@ print("Preparing the addons shared string file")
print("First, pull in the strings from the YAML file")
translation_strings = parseYAMLTranslationStrings(args.infile)
translation_strings = prune_lists_to_strings(translation_strings)
-# parseYAMLTranslationStrings gives a different ID than we want to use
-translation_strings = use_proper_id(translation_strings)
print("Then, write the strings to a .ts file")
tmp_path = tempfile.mkdtemp()
ts_file = os.path.join(tmp_path, "sharedAddonsStrings.ts")
-write_en_language(ts_file, translation_strings)
+write_en_language(ts_file, translation_strings, key_as_id=False)
print("Finally, convert the ts file into an xliff file")
os.system(f"{lconvert} -if ts -i {ts_file} -of xlf -o {args.outfile}")There was a problem hiding this comment.
Good call, thanks. I've updated this.
| content = json.load(file) | ||
|
|
||
| if not content: | ||
| print(f"No content found in: {filepath}") |
There was a problem hiding this comment.
Suggested change
| print(f"No content found in: {filepath}") | |
| sys.exit(f"No content found in: {filepath}") |
You can exit directly with the message (across the file).
There was a problem hiding this comment.
I had it that way initially, but moved to print combined with sys.exit(1) to match the style in the rest of this file.
flodolo
approved these changes
Nov 26, 2024
oskirby
reviewed
Dec 2, 2024
docs/Components/Addons/index.md
Outdated
| @@ -152,18 +152,19 @@ If you want to implement new add-ons, you need to follow these steps: | |||
|
|
|||
| 1. Create a manifest in a separate folder in the `addons` directory of the mozilla VPN repository. | |||
| 2. Read and follow the documentation for the add-on type you want to implement. | |||
| 3. Build the CMake project in the `addons` directory. | |||
| 3. If new shared strings were added, process them so that strings are shown in the addon: `python ./scripts/utils/generate_shared_addon_xliff.py -i ./addons/strings.yaml -o ./3rdparty/i18n/en/addons/strings.xliff` (This step is not necessary, strictly speaking. This script is what will be run as new strings are ingested into l10n repo, and once the new strings have done a round trip to the l10n repo and then back to the client repo, the string will show up in the addon as expected. Manually running this script as part of the build process prevents the brand new addon from showing "vpn.newStringCategory.newString" until this round trip happens.) | |||
There was a problem hiding this comment.
nit: could you please split this line and keep the file under 100-ish characters wide?
oskirby
approved these changes
Dec 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The “shared addons strings” for VPN (VPN PR, translations PR) had a bug - the string IDs in pontoon are of the form
CommonStringsSubtitle, and the intention wasvpn.commonStrings.subtitle. While this is non-standard (per the rest of our string IDs), it also provides an unlikely-but-real technical issue of a future clash -update.soonMessageandupdateSoon.messagefrom different parts of the YAML file would end up with the same ID ofCommonStringsUpdateSoonMessage.This PR does 3 things:
generate_shared_addons_xliff.pycheck_l10n_issues.pythat probably should have been there from the start - it would have caught this issue. This confirms that all shared strings from addons are present in the processed translation file.There is another PR on the l10n repo to update the current translation strings to this id format: mozilla-l10n/mozilla-vpn-client-l10n#494
The two PRs must be merged at the same time, so that the subsequent ingestion script run doesn't re-introduce the bad IDs and doesn't introduce the good IDs (without translations).
Reference
VPN-6749
Checklist